-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support Option<Vec> and Option<String> #14
base: main
Are you sure you want to change the base?
Conversation
src/rust_option.rs
Outdated
const _: () = { | ||
impl_option_ffi! { <T: OptionPtrTarget>, usize, &'static () } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here I define the partner struct (what was RustOption). So that I don't need a unique name for each different type, the definition is tucked away in a const. This means that we can't name this type outside of this block. The rest of the commit is removing all references to RustOption
outside and replacing it with <Option<T> as OptionFfi>::Target
to deal with this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does is_direct
correspond with needs_indirect_abi
? Or are they unrelated? I was a bit confused by the naming there.
How about Option<&str>
. Is that supported here? I see impl for Option<&String>
but not str.
) = &sig.ret | ||
{ | ||
write!(out, ")"); | ||
if let Some(ret) = &sig.ret { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is starting to look like it wants to be a match instead of if-let chain
src/rust_option.rs
Outdated
} | ||
|
||
/// SAFETY: ptr must be valid for 'a | ||
pub unsafe fn from_raw_double_repr( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does "raw double repr" mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
repr
is for *RustVec<T> -> Option<&Vec<T>>
and double_repr
is specifically for *RustVec<RustString>
-> Option<&Vec<String>>
(both the container and the inner type change)
However, I can also just use as _
magic casts on the pointers to get rid of both the repr and double_repr methods (just from_raw
) so I think I'm gonna do that instead
Hrmm is_direct was referring to the fact that I think that the code was actually magically passing T* anyway due to calling convention so my code was luckily working. I removed the
Option<&str> is not yet implemented because it is a fat pointer. Planning on doing it in a future diff though |
The main issue with supporting these structs is that they are both 3 pointers wide, while the existing code relied on Option being precisely 1 pointer wide (and thus only worked for pointer types). To resolve this, this PR has 2 commits:
Instead of using a single RustOption type to represent the C++ binding, using traits I defined different structs for the FFI representation of Option depending on T. For the old pointer types like &T and Box this is the old RustOption, but this now allows me to define new types (with a different size) to use for Option and Option
Actually implement Option and Option using the indirection above